Skip to content

Fix: some easy-to-fix problems in [Code scan] issues#7582

Open
Critsium-xy wants to merge 8 commits into
deepmodeling:developfrom
Critsium-xy:fix/code-scan-tier1
Open

Fix: some easy-to-fix problems in [Code scan] issues#7582
Critsium-xy wants to merge 8 commits into
deepmodeling:developfrom
Critsium-xy:fix/code-scan-tier1

Conversation

@Critsium-xy

@Critsium-xy Critsium-xy commented Jul 2, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

This PR fixes a batch of low-risk, localized issues reported by the recent Codex global repository scan (the "[Code scan]" issues). Each fix is a separate commit referencing its issue.

Fixes

Issue Fix
#7541 Correct syntax error (fdir suffix + s_dir → fdir + s_dir) in the RT-TDDFT projection tool
#7542 Repair empty else branch (add : no-op) in generate_orbital_mixstru.sh so bash -n passes
#7552 Reject one-past angular momentum index in TwoCenterTable::is_present (l <= → l <)
#7551 Copy the innermost dimension (size[2], not size[1]) when slicing 3D tensors
#7548 Delete OFDFT KEDF_Manager with scalar delete to match its allocation
#7572 Use the consistent SUPERLU_DIST32_ROOT variable in CI path exports
#7555 Read LATTICE_PARAMETER blocks consistently in the Multiwfn and pyabacus STRU parsers (partial)

Notes on scope:

Verification

  • Python files pass python -m py_compile.
  • The shell script passes bash -n.
  • C++ / CMake / CI changes are localized one-liners reviewed against surrounding idioms.

Critsium-xy and others added 5 commits July 2, 2026 14:34
@Critsium-xy @claude
fix(tools): correct syntax error in RT-TDDFT projection tool
54cc6c0 
Line 9 contained the invalid expression `fdir suffix + s_dir`, which
made projection.py unparseable. Use `fdir + s_dir` to match the working
example copy.

Closes deepmodeling#7541

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@Critsium-xy @claude
fix(tools): repair empty else branch in generate_orbital_mixstru.sh
64edf18 
`bash -n` failed on the empty `else` branch before `fi`. Add a `:`
no-op so the example script is syntactically valid.

Closes deepmodeling#7542

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@Critsium-xy @claude
fix(nao): reject one-past angular momentum index in TwoCenterTable
2cf3166 
index_map_ is allocated with final dimension length bra.lmax()+ket.lmax()+1,
so valid indices are 0..dim_size(6)-1. The bounds check used `l <=
dim_size(6)`, allowing a one-past read. Use `l < dim_size(6)` to match the
neighboring dimension checks.

Closes deepmodeling#7552

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@Critsium-xy @claude
fix(container): copy innermost dimension when slicing 3D tensors
4d6d6f3 
Each 3D row is placed at offset_out advancing by size[2], so the
contiguous copy length must also be size[2]. The copy used size[1],
corrupting non-cubic slices where size[1] != size[2].

Closes deepmodeling#7551

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@Critsium-xy @claude
fix(esolver): delete OFDFT KEDF_Manager with scalar delete
9e77036 
kedf_manager_ is allocated with scalar `new KEDF_Manager()`, but the
reinitialization path in before_all_runners() freed it with `delete[]`,
which is undefined behavior. Use scalar `delete` to match the allocation.

Closes deepmodeling#7548

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@Critsium-xy Critsium-xy changed the title Fix: batch of Tier-1 [Code scan] issues Fix: some easy-to-fix problems in [Code scan] issues Jul 2, 2026
@Growl1234

Growl1234 commented Jul 2, 2026 •
edited
Loading

Copy link
Copy Markdown

I fully disagree with the fix regarding #7568 and #7572; the reason, also what the real problem should lie in, has already been described simply under those issues.

  • Adding DEFAULT_MSG would only make FindMKL.cmake more fragile.
  • We should try to drop redundant environment variables in CI workflows if possible rather than only fix the known issue.

@Critsium-xy

Critsium-xy commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator Author

I fully disagree with the fix regarding #7568 and #7572; the reason, also what the real problem should lie in, has already been described simply under those issues.

  • Adding DEFAULT_MSG would only make FindMKL.cmake more fragile.
  • We should try to drop redundant environment variables in CI workflows if possible rather than only fix the known issue.

Okay, I will further investigate about both issues and this PR. This is only a draft now.

@Growl1234

Growl1234 commented Jul 2, 2026

Copy link
Copy Markdown

I'm having a deeper look about CMake cleanup these days, and the revision about FindMKL.cmake (and SetupCuSolverMp.cmake) is on my to-do list. In order to avoid duplication, would you agree with I keeping an eye on these files under this PR?

@Critsium-xy

Critsium-xy commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator Author

I'm having a deeper look about CMake cleanup these days, and the revision about FindMKL.cmake (and SetupCuSolverMp.cmake) is on my to-do list. In order to avoid duplication, would you agree with I keeping an eye on these files under this PR?

As you are trying to fix the thing about FindMKL.cmake, I will remove the commits about this file in my PR.

@kirk0830

kirk0830 commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator

@Critsium-xy hi, because there are also commits to push from @QuantumMisaka side, I suggest pick the fix related with abacuslite out from this PR, in case of time consuming conflict-solving.
Never mind, the developers of abacuslite are still active ;)

@Critsium-xy @claude
fix(ci): use consistent SuperLU_DIST32_ROOT variable in path exports
12f66ac 
LD_LIBRARY_PATH, PKG_CONFIG_PATH and CPATH referenced the misspelled
SUPERLU32_DIST_ROOT while CMAKE_PREFIX_PATH used SUPERLU_DIST32_ROOT (the
name defined in Dockerfile.intel). Align the path exports to
SUPERLU_DIST32_ROOT so SuperLU_DIST is not silently omitted.

Closes deepmodeling#7572

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@Critsium-xy Critsium-xy force-pushed the fix/code-scan-tier1 branch from dc1f11a to 1c0d273 Compare July 3, 2026 02:14
@Critsium-xy

Critsium-xy commented Jul 3, 2026

Copy link
Copy Markdown
Collaborator Author

@Critsium-xy hi, because there are also commits to push from @QuantumMisaka side, I suggest pick the fix related with abacuslite out from this PR, in case of time consuming conflict-solving. Never mind, the developers of abacuslite are still active ;)

Ok I will quickly do that.

@Critsium-xy @claude
fix(io): read LATTICE_PARAMETER blocks consistently in STRU parsers
db5774d 
The Multiwfn and pyabacus STRU parsers checked for a LATTICE_PARAMETER
block but then read blocks['LATTICE_PARAMETERS'] with an extra S, and
treated the list of lines as a string. Read
blocks['LATTICE_PARAMETER'][0].split() to match the neighboring
LATTICE_CONSTANT idiom, so STRU files using LATTICE_PARAMETER parse
instead of raising KeyError.

The identical fix for the ASE AbacusLite parser is intentionally left out
of this PR and will be handled separately.

Refs deepmodeling#7555

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@Critsium-xy Critsium-xy force-pushed the fix/code-scan-tier1 branch from 1c0d273 to db5774d Compare July 3, 2026 02:19
@mohanchen mohanchen added Bugs Bugs that only solvable with sufficient knowledge of DFT Refactor Refactor ABACUS codes labels Jul 3, 2026
@Critsium-xy Critsium-xy marked this pull request as ready for review July 3, 2026 07:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

Bugs Bugs that only solvable with sufficient knowledge of DFT Refactor Refactor ABACUS codes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Critsium-xy @Growl1234 @kirk0830 @mohanchen